Skip to content

feat: remove linting rules for composite types#2658

Merged
Noroth merged 10 commits intomainfrom
ludwig/eng-8724-add-support-for-abstract-types-in-protographic
Mar 18, 2026
Merged

feat: remove linting rules for composite types#2658
Noroth merged 10 commits intomainfrom
ludwig/eng-8724-add-support-for-abstract-types-in-protographic

Conversation

@Noroth
Copy link
Copy Markdown
Contributor

@Noroth Noroth commented Mar 17, 2026

This PR removes the linting rules blocking the generation of message types for composite types in the field set of @requires

Summary by CodeRabbit

  • New Features

    • Expose cost controls: @cost and @listsize support across tooling, router, and platform (headers, metrics, and config).
    • Add "recompose" operation/CLI to trigger graph recomposition and report composition/deployment results.
  • Documentation

    • New architecture and cost-control docs and examples; docs site page for cost control.
  • Tests

    • Comprehensive tests added for cost and recompose flows.
  • Refactor

    • Removed @requires directive validation feature and related checks; streamlined validation rules.
  • Chores

    • Removed associated validation tests and bumped package versions.

Checklist

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 17, 2026

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Removed the @requires directive validation from protographic: deleted the SelectionSetValidationVisitor, removed requires-related imports and logic from sdl-validation-visitor, and deleted corresponding tests. Linting rules were updated to omit the disallowAbstractTypesForRequires rule.

Changes

Cohort / File(s) Summary
Validation System
protographic/src/sdl-validation-visitor.ts, protographic/src/selection-set-validation-visitor.ts
Removed requires-directive validation: deleted SelectionSetValidationVisitor, removed validateRequiresDirective and requires-related imports/fields, and removed the disallowAbstractTypesForRequires rule from active rules.
Tests
protographic/tests/sdl-validation/01-basic-validation.test.ts
Removed three tests that asserted errors for invalid @requires usage (abstract type in requires, invalid selection syntax, and unclosed brace).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.54%. Comparing base (0b76c8c) to head (001e1ac).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2658       +/-   ##
===========================================
+ Coverage   41.37%   89.54%   +48.16%     
===========================================
  Files         783       20      -763     
  Lines      111952     4360   -107592     
  Branches     8573     1199     -7374     
===========================================
- Hits        46325     3904    -42421     
+ Misses      65264      456    -64808     
+ Partials      363        0      -363     
Files with missing lines Coverage Δ
protographic/src/sdl-validation-visitor.ts 87.05% <100.00%> (ø)

... and 802 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Noroth Noroth marked this pull request as ready for review March 17, 2026 10:50
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@protographic/src/sdl-validation-visitor.ts`:
- Line 146: The change removed all `@requires` validation by replacing the
lintingRules array; restore a minimal requires validator rather than dropping it
entirely: ensure this.lintingRules includes a requiresRule (or reintroduce a
requiresRule function) alongside objectTypeRule, listTypeRule, providesRule, and
resolverContextRule, where requiresRule enforces `@requires` syntax and dependency
integrity but does NOT block non-composite types—only remove the
abstract/composite-type restriction within requiresRule's logic so it validates
field presence and correct referenced dependencies without forbidding valid
concrete `@requires` usages.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f44bc2e6-fcd3-4c3e-8d91-fdeae307c89d

📥 Commits

Reviewing files that changed from the base of the PR and between f24e435 and 6b31cf8.

📒 Files selected for processing (3)
  • protographic/src/sdl-validation-visitor.ts
  • protographic/src/selection-set-validation-visitor.ts
  • protographic/tests/sdl-validation/01-basic-validation.test.ts
💤 Files with no reviewable changes (2)
  • protographic/tests/sdl-validation/01-basic-validation.test.ts
  • protographic/src/selection-set-validation-visitor.ts

Comment thread protographic/src/sdl-validation-visitor.ts
Aenimus and others added 6 commits March 18, 2026 10:24
 - wgc@0.111.0
 - @wundergraph/cosmo-connect@0.136.0
 - controlplane@0.206.0
 - @wundergraph/protographic@0.18.1
 - @wundergraph/cosmo-shared@0.44.5
 - studio@0.161.8
Integrate static and dynamic cost calculation into the router. 
Expose the static (estimated) cost value in the modules.
Expose both costs via telemetry and response headers.

Metrics for costs should be enabled individually in the corresponding telemetry section.

Composition was reviewed in another PR and merged here.

End-to-end benchmark for a big query to measure the impact of cost control:

     │ SequentialBig │     SequentialBigCostAnalysis      │
     │    sec/op     │   sec/op     vs base               │
*-14     584.1µ ± 0%   586.7µ ± 0%  +0.45% (p=0.000 n=40)

     │ SequentialBig │      SequentialBigCostAnalysis      │
     │      B/s      │     B/s       vs base               │
*-14    5.527Mi ± 0%   5.503Mi ± 0%  -0.43% (p=0.000 n=40)

     │ SequentialBig │   SequentialBigCostAnalysis    │
     │     B/op      │     B/op      vs base          │
*-14    456.1Ki ± 0%   456.1Ki ± 0%  ~ (p=0.822 n=40)

     │ SequentialBig │     SequentialBigCostAnalysis      │
     │   allocs/op   │  allocs/op   vs base               │
*-14     6.444k ± 0%   6.452k ± 0%  +0.12% (p=0.000 n=40)

Fixes ENG-8844
Fixes ENG-8986

Co-authored-by: Aenimus <47415099+Aenimus@users.noreply.github.com>
Co-authored-by: StarpTech <deusdustin@gmail.com>
 - wgc@0.112.0
 - @wundergraph/composition@0.54.0
 - @wundergraph/cosmo-connect@0.137.0
 - controlplane@0.207.0
 - @wundergraph/protographic@0.18.2
 - router@0.294.0
 - @wundergraph/cosmo-shared@0.45.0
 - studio@0.161.9
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 18, 2026

❌ Internal Query Planner CI failed to run.

@Noroth Noroth merged commit 3426dd3 into main Mar 18, 2026
10 checks passed
@Noroth Noroth deleted the ludwig/eng-8724-add-support-for-abstract-types-in-protographic branch March 18, 2026 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants